Update aggregation layers documentation #9135
Conversation
Pessimistress
force-pushed
the
x/aggregation-percentile
branch
from
2cd0deb
to
539eff9
Compare
Pessimistress
force-pushed
the
x/aggregation-docs
branch
from
89954ee
to
ecea16a
Compare
There was a problem hiding this comment.
This looks like a big improvement.
Longer term wishlist:
- It would be really nice to emphasize that we have an independently useful aggregator (can used without layers) if that is the case.
- Also would love to see us use more data table oriented terminology (e.g columns instead of attributes)
|
|
||
| This layer creates `AttributeManager` and makes it available for its subclasses. Any aggregation layer can add attributes to the `AttributeManager` and retrieve them using `getAttributes` method. This enables using `AttributeManager`'s features and optimization for using attributes. Also manual iteration of `data` prop can be removed and attributes can be directly set on GPU aggregation models or accessed directly for CPU aggregation. | ||
| Returns a string that indicates the type of aggregator that this layer uses, for example `'gpu'`. This method is invoked with each `updateState` lifecycle. If the type string does not match its previous value, the existing aggregator will be disposed and recreated. |
There was a problem hiding this comment.
Can this be reworded somehow, the words type and method are a little confusing...
| Returns a string that indicates the type of aggregator that this layer uses, for example `'gpu'`. This method is invoked with each `updateState` lifecycle. If the type string does not match its previous value, the existing aggregator will be disposed and recreated. | |
| Returns a string that indicates the type of aggregator that this layer uses, for example `'gpu'`. This aggregator is invoked with each `updateState` lifecycle. If the type string does not match its previous value, the existing aggregator will be disposed and recreated. |
There was a problem hiding this comment.
The phrase "this method" refers to getAggregatorType(), so it doesn't make sense to change it to "this aggregator". This sentence is clearly not explaining the process well, but I don't know how to improve it.
There was a problem hiding this comment.
The phrase "this method" refers to
getAggregatorType(), so it doesn't make sense to change it to "this aggregator". This sentence is clearly not explaining the process well, but I don't know how to improve it.
Sounds good, just thinking:
- Maybe align the wording
getAggregationMethod()then talking about "this method" makes a bit more sense. - Maybe the term "lifecycle" here may not be that helpful?
- Or how about:
the aggregation selected by 'getAggregationMethod()' is run during each 'updateState' call. - Or:
the selected aggregation is run during each 'updateState' call.
|
|
||
| During update state, Subclasses of `AggregationLayer` must first call 'super.updateState()', which calls | ||
| #### `onAttributeChange` {#onattributechange} |
There was a problem hiding this comment.
Thought for future: Could this be a base layer callback?
There was a problem hiding this comment.
We can probably require Aggregtor to be defined with a map of dependencies/update triggers. I'll play with some options.
|
|
||
| _Aggregation_ is a 2-step process: | ||
|
|
||
| 1. **Sort**: Group a collection of _data points_ by some property into bins_. |
There was a problem hiding this comment.
Thought for future,: maybe we could define two composable interfaces for these two steps?
There was a problem hiding this comment.
WebGLAggregator does the two steps in one pass.
There was a problem hiding this comment.
WebGLAggregator does the two steps in one pass.
I see. Perhaps one could generalize the code so it could just one of them somehow. For another time...
| ```ts | ||
| aggregator.setProps({ | ||
| pointCount: 10000, | ||
| attributes: {...}, |
There was a problem hiding this comment.
One day we should call this columns. Not sure if we could go that far now....
There was a problem hiding this comment.
These are the same format as props.data.attributes and we have a bunch of classes/types along that line such as AttributeManager, BinaryAttribute etc. At least for now I don't think we should add more concepts to the mix.
There was a problem hiding this comment.
Makes sense. But I would love to give this an overhaul in deck.gl v10 as part of deep Arrow integration etc.
Pessimistress
force-pushed
the
x/aggregation-percentile
branch
2 times, most recently
from
cccee98
to
d3a5457
Compare
Pessimistress
force-pushed
the
x/aggregation-docs
branch
from
ecea16a
to
2adb243
Compare
Pessimistress
force-pushed
the
x/aggregation-docs
branch
from
2adb243
to
9b6c6ef
Compare
For #9056
Change List